Generate different DocumentIds for duplicate generators #64729
Conversation
We've seen a few places in the wild where a customer ends up with a generator installed twice into their project. This was causing an issue with how we generate DocumentIds for generated documents: we compute that as a stable hash over some pieces of information so that way we geneate consistent IDs in different processes, and a generated file that disappears due to an edit and comes back can also have the same ID. The hashing algorithm for a DocumentId included the generator's type name and the generator's assembly name, but if you have two generators with the same assembly name we'd collide. This could happen if you have two different generators with the same name, or your project file is messed up and causing a generator to be added from two different paths at once. This latter case isn't a scenario we support, but this change at least ensures Roslyn won't be throwing exceptions which ends up requiring investigation. Related to dotnet#56619, but doesn't entirely close it, a duplicate generator producing duplicate files will still be confusing. This may also address dotnet#57660 but the belief is there's a race condition there too, as the dumps I've seen don't show duplicate references in the project.
| @@ -91,6 +91,28 @@ public async Task WithReferencesMethodCorrectlyUpdatesRunningGenerators() | |||
| Assert.Single((await project.GetRequiredCompilationAsync(CancellationToken.None)).SyntaxTrees); | |||
| } | |||
|
|
|||
| // We only run this test on Release, as the compiler has asserts that trigger in Debug that the type names probably shouldn't be the same. | |||
There was a problem hiding this comment.
@chsienki This is because of the assert that's on the GeneratorDriver constructor...not sure if we want to change it or not.
There was a problem hiding this comment.
yeah, tahts' a bad assert. it should either go or throw.
| @@ -346,7 +346,7 @@ public async ValueTask RefreshFileAsync(CancellationToken cancellationToken) | |||
| else | |||
| { | |||
| // The file isn't there anymore; do we still have the generator at all? | |||
| if (project.AnalyzerReferences.Any(a => a.GetGenerators(project.Language).Any(static (g, self) => SourceGeneratorIdentity.GetGeneratorAssemblyName(g) == self._documentIdentity.Generator.AssemblyName, this))) | |||
| if (project.AnalyzerReferences.Any(a => a.FullPath == _documentIdentity.Generator.AssemblyPath)) | |||
There was a problem hiding this comment.
is == ok on filepaths on windows?
There was a problem hiding this comment.
Generally it can be iffy, although in this case the AssemblyPath came from the analzyer reference's path in the first place. I'm going to leave this as is, since at some point this code needs to migrate to potentially run in VS for Mac too, where such a case insensitive comparison would actually be problematic, and no reason to add the extra overhead of the insensitive comparison if not needed.
ghost
added this to the 
|
@jasonmalinowski This seems like a 17.5 candidate right? Not 17.4. If there are additional reports of such problems, we can certainly service it. What do you think? |
|
@arkalyanms I haven't seen any specific evidence this is impactful for users other than the case where the projects are all broken, so at this point I'm keeping this 17.5 since there's some risk to this change, and it'd be good to kick the tires here. I did write this branch against our 17.4 branch though so if that situation changes it'll be easy to move back to 17.4. |
We've seen a few places in the wild where a customer ends up with a generator installed twice into their project. This was causing an issue with how we generate DocumentIds for generated documents: we compute that as a stable hash over some pieces of information so that way we geneate consistent IDs in different processes, and a generated file that disappears due to an edit and comes back can also have the same ID.
The hashing algorithm for a DocumentId included the generator's type name and the generator's assembly name, but if you have two generators with the same assembly name we'd collide. This could happen if you have two different generators with the same name, or your project file is messed up and causing a generator to be added from two different paths at once. This latter case isn't a scenario we support, but this change at least ensures Roslyn won't be throwing exceptions which ends up requiring investigation.
Related to #56619, but doesn't entirely close it, a duplicate generator producing duplicate files will still be confusing. This may also address
#57660 but the belief is there's a race condition there too, as the dumps I've seen don't show duplicate references in the project.